Fix: jest-worker: should not expose `.default` babel interop by anje123 · Pull Request #10623 · jestjs/jest

anje123 · 2020-10-11T18:06:35Z

Summary

import a from jest-worker of course, works fine, because it's using babel on both ends.
However, const b = require('jest-worker') results in a !== b, and to get at the proper value, you need to do b.default.
Fixes: #5803

Test plan

I will appreciate some guide to evaluate, if this is a valid change and if it solves the problem.

SimenB · 2020-10-11T18:38:14Z

@anje123 you need to change

https://github.com/facebook/jest/blob/e79744e12dc18d25be8e80c429e870303b7c8c18/packages/jest-runner/src/__tests__/testRunner.test.ts#L14-L24

and

https://github.com/facebook/jest/blob/e79744e12dc18d25be8e80c429e870303b7c8c18/packages/jest-haste-map/src/__tests__/index.test.js#L22-L32

Also please update the changelog

ljharb · 2020-10-12T02:04:27Z


  it('distributes work across workers', () => {
-    const jestWorker = require('jest-worker');
+    const jestWorker = require('jest-worker').Worker;


Does this make this PR a breaking change?

@ljharb Yes it needed to be changed, if there is a better way, i will appreciate some guide

The solution is to change the "main" of jest-worker to be untranspiled CJS, and to require the transpiled file. That gives you precise control over the exports.

@ljharb i dont understand your explanation, can u break it down a bit

I'm saying, don't use a babel-transpiled file that uses export syntax - use a normal node CJS file that uses require and module.exports.

I don't want the extra file/work that is maintaining a "separate" CJS entrypoint, so the change I'll be accepting is allowing CJS to be const {Worker} = require('jest-worker'); (i.e. skipping .default), not removing __esModule from the exported object

That unfortunately means it'll be a breaking change, so this won't land until Jest 27, but I don't think it's worth the maintenance burden to add any extra entrypoints etc

That's very unfortunate imo; it adds a UX burden for all consumers to avoid a (in my experience) minor maintenance burden.

That said, will ESM consumers also have the same experience as CJS consumers - ie, no default, just a named export?

That said, will ESM consumers also have the same experience as CJS consumers - ie, no default, just a named export?

Yes. Or rather, ESM via TypeScript/Babel etc - native ESM will have .default.Worker I believe due to Node's CJS->ESM handling. I haven't verified, though

SimenB · 2020-10-12T08:39:55Z

@anje123 this is looking good! Could you merge in master and resolve the conflict, plus update the readme to reflect the new syntax?

SimenB

Thanks! I've added it to the Jest 27 milestone so we remember to land it when we start making breaking changes.

SimenB · 2020-10-12T09:19:59Z


 ### Fixes
+- `[jest-worker, jest-haste-map, jest-runner, jest-reporters]` Fix: jest-worker: should not expose `.default` babel interop ([#10623] (https://github.com/facebook/jest/pull/10623))
+- `[jest-runner, jest-runtime]` fix: `require.main` undefined with `createRequire()` ([#10610](https://github.com/facebook/jest/pull/10610))


this and the 3 below should not be here

SimenB · 2020-10-12T09:57:57Z


 ### Features

+- `[jest-worker, jest-haste-map, jest-runner, jest-reporters]` Fix: jest-worker: should not expose `.default` babel interop ([#10623] (https://github.com/facebook/jest/pull/10623))


sorry, this one should be under master 😅

If you tick this box in the lower right I can fix all of these little nits myself

@SimenB i cant see this option

hmm, might be it needs to be enabled by repoo admins. @mkcode might know? Giving me push permission to the MLH fork should also fix it so I'm able to push fixups to your PRs

SimenB · 2020-12-05T15:05:28Z

@anje123 could you rebase this, please? 🙂

github-actions · 2021-05-10T23:06:59Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the cla signed label Oct 11, 2020

anje123 mentioned this pull request Oct 11, 2020

jest-worker: should not expose .default babel interop #5803

Closed

anje123 changed the title ~~[WIP] Fix: jest-worker: should not expose .default babel interop~~ Fix: jest-worker: should not expose .default babel interop Oct 11, 2020

ljharb reviewed Oct 12, 2020

View reviewed changes

SimenB added the 💣 Breaking change label Oct 12, 2020

SimenB added this to the Jest 27 milestone Oct 12, 2020

SimenB approved these changes Oct 12, 2020

View reviewed changes

SimenB reviewed Oct 12, 2020

View reviewed changes

SimenB approved these changes Oct 12, 2020

View reviewed changes

anje123 force-pushed the fix-export-default-worker branch from a4d87e2 to def3b01 Compare December 5, 2020 16:42

flozender and others added 6 commits December 5, 2020 18:06

default export to named export - JestWorker

9c9895e

Fix-export-default-worker

f7743bd

Fix:jest-worker:should-not-expose--babel-interop

9b35d6c

Fix:jest-worker:should-not-expose--babel-interop

eb16d3f

Fix:jest-worker:should-not-expose--babel-interop

ea39684

fix

c93aa46

anje123 force-pushed the fix-export-default-worker branch from def3b01 to c93aa46 Compare December 5, 2020 18:08

SimenB added 3 commits December 5, 2020 19:32

Update CHANGELOG.md

9d481db

Merge branch 'master' into fix-export-default-worker

e012c40

Update CHANGELOG.md

34e3bdd

SimenB merged commit 4418e76 into jestjs:master Dec 6, 2020

SimenB deleted the fix-export-default-worker branch December 6, 2020 10:30

SimenB mentioned this pull request Dec 7, 2020

SyntaxError: Cannot use import statement outside a module, with TypeScript and ES Modules #9860

Closed

github-actions Bot locked as resolved and limited conversation to collaborators May 10, 2021


		### Features

		- `[jest-worker, jest-haste-map, jest-runner, jest-reporters]` Fix: jest-worker: should not expose `.default` babel interop ([#10623] (https://github.com/facebook/jest/pull/10623))

Uh oh!

Conversation

anje123 commented Oct 11, 2020

Summary

Test plan

Uh oh!

SimenB commented Oct 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Oct 12, 2020

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimenB commented Dec 5, 2020

Uh oh!

github-actions Bot commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SimenB Oct 12, 2020 •

edited

Loading

SimenB Oct 12, 2020 •

edited

Loading